Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request migrates DB-API DDL tests from system tests to mock server tests, introducing a comprehensive suite in test_dbapi_ddl.py and removing redundant tests from test_dbapi.py. Additionally, test_autocommit_with_json_data is refactored to use an existing table. Feedback on the changes highlights a potential state leak in test_autocommit_with_json_data where self._conn.autocommit is modified but not restored, which could affect subsequent tests sharing the same connection.
Replaces live-instance DDL system tests in `test_dbapi.py` with fast, deterministic in-memory mock server tests in `test_dbapi_ddl.py`. Previously, `test_dbapi.py` dynamically created and dropped ephemeral tables (e.g. `Singers`, `DdlExecute`, `DdlExecuteMany`, `JsonDetails`) against live Cloud Spanner instances to verify client-side DDL buffering and flushing. Each table creation/drop triggered a long-running operation (LRO), adding ~1.5 to 2.0 minutes of latency and causing test flakiness. Changes: - Add `test_dbapi_ddl.py` in `tests/mockserver_tests/` with 9 mock server tests covering immediate autocommit DDL execution, non-autocommit DDL buffering, multi-statement DDL batches, DDL flushing before subsequent queries and DML, and client-side rejection of DDL in `executemany()`. - Remove 7 redundant DDL tests from `tests/system/test_dbapi.py`. - Update `test_autocommit_with_json_data` in `test_dbapi.py` to reuse the existing `all_types` table with row cleanup instead of creating and dropping a dedicated table. - Eliminate all dynamic table churn in DB-API system tests, saving ~1.5 to 2.0 minutes in standard CI while executing the replacement mock tests in < 1 second.
olavloite
force-pushed
the
spanner-replace-dbapi-ddl-sys-tests
branch
from
October 14, 2026 07:33
36fbef3 to
5c1c93b
Compare
olavloite
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
parthea
approved these changes
Oct 17, 2026
| self.assertEqual(1, len(execute_requests)) | ||
| self.assertEqual(dml_statement, execute_requests[0].sql) | ||
|
|
||
| connection.commit() |
Contributor
There was a problem hiding this comment.
Is there any value in moving this commit higher up above the asserts? Gemini spotted a testing gap that we should verify both that the DDL flushed before the query/DML and that commit() didn't re-flush it
Here is the full diff suggested by Gemini
--- a/packages/google-cloud-spanner/tests/mockserver_tests/test_dbapi_ddl.py
+++ b/packages/google-cloud-spanner/tests/mockserver_tests/test_dbapi_ddl.py
@@ -126,10 +126,13 @@ class TestDbapiDdl(MockServerTestBase):
cursor.execute(dml_statement)
self.assertEqual(1, len(self.database_admin_service.requests))
- request = self.database_admin_service.requests[0]
- self.assertEqual(self.database.name, request.database)
- self.assertEqual([ddl_statement], request.statements)
self.assertEqual(1, cursor.rowcount)
+ connection.commit()
+
+ self.assertEqual(1, len(self.database_admin_service.requests))
+ request = self.database_admin_service.requests[0]
+ self.assertEqual(self.database.name, request.database)
+ self.assertEqual([ddl_statement], request.statements)
+
execute_requests = [
req
for req in self.spanner_service.requests
if isinstance(req, ExecuteSqlRequest)
]
self.assertEqual(1, len(execute_requests))
self.assertEqual(dml_statement, execute_requests[0].sql)
-
- connection.commit()
finally:
connection.close()
@@ -163,10 +166,13 @@ class TestDbapiDdl(MockServerTestBase):
cursor.executemany(dml_statement, [()])
self.assertEqual(1, len(self.database_admin_service.requests))
- request = self.database_admin_service.requests[0]
- self.assertEqual(self.database.name, request.database)
- self.assertEqual([ddl_statement], request.statements)
self.assertEqual(1, cursor.rowcount)
+ connection.commit()
+
+ self.assertEqual(1, len(self.database_admin_service.requests))
+ request = self.database_admin_service.requests[0]
+ self.assertEqual(self.database.name, request.database)
+ self.assertEqual([ddl_statement], request.statements)
+
batch_requests = [
req
for req in self.spanner_service.requests
if isinstance(req, ExecuteBatchDmlRequest)
]
self.assertEqual(1, len(batch_requests))
self.assertEqual(dml_statement, batch_requests[0].statements[0].sql)
-
- connection.commit()
finally:
connection.close()
@@ -198,9 +204,10 @@ class TestDbapiDdl(MockServerTestBase):
cursor.execute("SELECT 1")
self.assertEqual(1, len(self.database_admin_service.requests))
- request = self.database_admin_service.requests[0]
- self.assertEqual(self.database.name, request.database)
- self.assertEqual([ddl_statement], request.statements)
connection.commit()
+
+ self.assertEqual(1, len(self.database_admin_service.requests))
+ request = self.database_admin_service.requests[0]
+ self.assertEqual(self.database.name, request.database)
+ self.assertEqual([ddl_statement], request.statements)
finally:
connection.close()
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces live-instance DDL system tests in
test_dbapi.pywith fast, deterministic in-memory mock server tests intest_dbapi_ddl.py.Previously,
test_dbapi.pydynamically created and dropped ephemeral tables (e.g.Singers,DdlExecute,DdlExecuteMany,JsonDetails) against live Cloud Spanner instances to verify client-side DDL buffering and flushing. Each table creation/drop triggered a long-running operation (LRO), adding ~1.5 to 2.0 minutes of latency and causing test flakiness.Changes:
test_dbapi_ddl.pyintests/mockserver_tests/with 9 mock server tests covering immediate autocommit DDL execution, non-autocommit DDL buffering, multi-statement DDL batches, DDL flushing before subsequent queries and DML, and client-side rejection of DDL inexecutemany().tests/system/test_dbapi.py.test_autocommit_with_json_dataintest_dbapi.pyto reuse the existingall_typestable with row cleanup instead of creating and dropping a dedicated table.